Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: show warning on 431 response #9324

Merged
merged 3 commits into from
Aug 10, 2022

Conversation

sapphi-red
Copy link
Member

Description

image
image

Show warnings when HPE_HEADER_OVERFLOW error happened. Currently the server does not output any errors or warnings.

close #547

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@sapphi-red sapphi-red added enhancement New feature or request p2-nice-to-have Not breaking anything but nice to have (priority) labels Jul 23, 2022
Comment on lines 192 to 201
server.on('clientError', (err, socket) => {
if ((err as any).code === 'HPE_HEADER_OVERFLOW') {
if (!socket.writableEnded) {
socket.end(
'HTTP/1.1 431 Request Header Fields Too Large\r\n\r\n' +
'Request Header was too large. Node.js limits request header size. ' +
'Use https://nodejs.org/api/cli.html#--max-http-header-sizesize to change max header size.'
)
}
logger.warn(
colors.yellow(
'Server / WS server responded with "431 Request Header Fields Too Large." ' +
'Node.js limits request header size and the request was dropped. ' +
'Use https://nodejs.org/api/cli.html#--max-http-header-sizesize to change max header size.'
)
)
}
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a bit specific that we only handle this specific error, maybe we should log the err generically instead? (not sure if that provides any message).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clientError event seems to be called with 400 and 431 (this one) but I'm not sure if we should output a warning for 400 too.

The err looks like this if it is output directly.
image

Maybe something like this is better?

Suggested change
server.on('clientError', (err, socket) => {
if ((err as any).code === 'HPE_HEADER_OVERFLOW') {
if (!socket.writableEnded) {
socket.end(
'HTTP/1.1 431 Request Header Fields Too Large\r\n\r\n' +
'Request Header was too large. Node.js limits request header size. ' +
'Use https://nodejs.org/api/cli.html#--max-http-header-sizesize to change max header size.'
)
}
logger.warn(
colors.yellow(
'Server / WS server responded with "431 Request Header Fields Too Large." ' +
'Node.js limits request header size and the request was dropped. ' +
'Use https://nodejs.org/api/cli.html#--max-http-header-sizesize to change max header size.'
)
)
}
})
server.on('clientError', (err, socket) => {
const code = (err as any).code
if (code === 'HPE_HEADER_OVERFLOW') {
logger.warn(
colors.yellow(
'Server / WS server responded with an error. ' +
`${err.message} (code: ${code})`
)
)
}
})

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the custom error message here is useful. Maybe something in between this two?
Another option is now that you started a troubleshooting page, we could start adding these longer explanations there, linking to it from the error instead of needing these messages that with time could add up to the size in Vite.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation @sapphi-red. I think the original PR makes sense then given two errors only happen here. It's strange that Node gives a cryptic error though, and my concern is with patak too that this increases the size for a very specific case.

I like the idea of putting this in the troubleshooting page too, we could probably elaborate more there too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds nice. I've move the details to troubleshooting page 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could discuss a bit in today's meeting. We could start moving other messages to the new guide, and add a section in the CONTRIBUTING guide to guide other contributors to do the same from now on. Maybe these should be in a section that only list errors, like https://yarnpkg.com/advanced/error-codes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, how React team built their error code system: https://reactjs.org/blog/2016/07/11/introducing-reacts-error-code-system.html

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @sapphi-red ,in the above code, the socket is not closed, in my case, it will hang up the webpage untill timeout reached. Could you please help me review this pull request? #9816

@sapphi-red sapphi-red force-pushed the feat/show-status-431-warning branch from da9bd34 to 45fcf23 Compare July 29, 2022 12:34
docs/guide/troubleshooting.md Outdated Show resolved Hide resolved
docs/guide/troubleshooting.md Outdated Show resolved Hide resolved
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
@patak-dev patak-dev merged commit e8b61bb into vitejs:main Aug 10, 2022
@sapphi-red sapphi-red deleted the feat/show-status-431-warning branch August 11, 2022 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Unexpected response code 431 in Vite Websocket
5 participants